Skip to content

Thales card support#125

Merged
mrts merged 1 commit into
web-eid:mainfrom
metsma:thales
Aug 24, 2025
Merged

Thales card support#125
mrts merged 1 commit into
web-eid:mainfrom
metsma:thales

Conversation

@metsma

@metsma metsma commented Apr 6, 2025

Copy link
Copy Markdown
Contributor

IB-8171

Signed-off-by: Raul Metsma raul@metsma.ee

@metsma
metsma force-pushed the thales branch 3 times, most recently from a47ad42 to 77fdd42 Compare April 7, 2025 07:09
@metsma
metsma force-pushed the thales branch 3 times, most recently from af2527a to 9d0c75f Compare May 29, 2025 09:37
@metsma
metsma force-pushed the thales branch 4 times, most recently from 1ddfbbb to e517837 Compare June 30, 2025 20:33
@metsma
metsma force-pushed the thales branch 6 times, most recently from c0a5df5 to 1b0e367 Compare July 18, 2025 08:05
@mrts
mrts self-requested a review July 25, 2025 18:30
@metsma
metsma force-pushed the thales branch 9 times, most recently from 9fff0ce to 38f9454 Compare July 27, 2025 21:34
Comment thread src/electronic-ids/pcsc/FinEID.cpp Outdated
Comment thread src/electronic-ids/pcsc/FinEID.hpp Outdated
Comment thread src/electronic-ids/TLV.hpp
Comment thread src/electronic-ids/TLV.hpp
Comment thread lib/libpcsc-cpp/src/SmartCard.cpp Outdated

void getResponseWithLE(ResponseApdu& response, byte_vector command) const
{
size_t pos = command.size() <= 5 ? 4 : 5 + command[4];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's follow defensive programming principles and protect against out of bounds writes.

Consider the following two cases:

Short command:

std::vector<uint8_t> cmd = {0x00,0x84,0x00,0x00};  // size()==4
size_t pos = cmd.size() <= 5 ? 4 : 5+cmd[4];  // pos==4
cmd[pos] = 0x08;  // write past end

Invalid Lc:

std::vector<uint8_t> cmd = {0x00,0xD0,0x00,0x00,0x10,  // Lc = 16
                            0xAA,0xBB,0xCC,0xDD,0xEE};  // only 5 data bytes, size()==10
size_t pos = 5 + cmd[4];  // 5 + 0x10 = 21
cmd[pos] = 0x08;  // write past end

We should throw cleanly and not write past end in these cases.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metsma correctly pointed out that the command byte vector is always converted from a CommandApdu that internally guarantees that Lc is valid.

So the issue is actually more about type safety - getResponseWithLE() and transitively transmitBytes() should use a CommandApdu parameter, not a raw byte vector, something like this:

    void getResponseWithLE(ResponseApdu& response, const CommandApdu& command) const
    {
        ...
    }

    ResponseApdu transmitBytes(const CommandApdu& command) const
    {
        byte_vector commandBytes = command;
        byte_vector responseBytes(ResponseApdu::MAX_SIZE, 0);
        auto responseLength = DWORD(responseBytes.size());

        SCard(Transmit, cardHandle, &_protocol, commandBytes.data(), DWORD(commandBytes.size()),
              nullptr, responseBytes.data(), &responseLength);

        auto response = toResponse(std::move(responseBytes), responseLength);

        if (response.sw1 == ResponseApdu::WRONG_LE_LENGTH) {
            getResponseWithLE(response, command);
        }
        ...
    }

@metsma
metsma force-pushed the thales branch 2 times, most recently from 79ceae6 to e7050c0 Compare August 1, 2025 07:42
@metsma
metsma force-pushed the thales branch 14 times, most recently from f02342c to a40cdf0 Compare August 4, 2025 06:02
@metsma
metsma force-pushed the thales branch 2 times, most recently from 87e9b98 to 2849ce1 Compare August 11, 2025 11:55
IB-8171

Signed-off-by: Raul Metsma <raul@metsma.ee>

#include <algorithm>

using namespace std::string_literals;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be top level as it leaks everywhere the header is used, let's move it into readBinary() scope where it used.

@mrts
mrts merged commit dfb29b8 into web-eid:main Aug 24, 2025
9 checks passed
@metsma
metsma deleted the thales branch August 25, 2025 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants